Richfr/ Add windowsAddress support to WslcCreateContainer() API#40037
Richfr/ Add windowsAddress support to WslcCreateContainer() API#400371wizkid wants to merge 20 commits intofeature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the WSLC Client SDK’s container port-mapping path to support custom host binding addresses (including IPv6) instead of always binding to 127.0.0.1, and relaxes/updates validation around the windowsAddress field.
Changes:
- Convert
windowsAddress(IPv4/IPv6) into a string binding address forWSLCPortMappingduring container creation. - Add input validation for
windowsAddress->ss_familyinWslcSetContainerSettingsPortMappings.
There was a problem hiding this comment.
Almost certainly needs a test update to prevent a negative test from failing when it passes in an address. Not sure if we can expect IPv6 everywhere, but at least passing in 127.0.0.1 (or maybe IPv6 loopback works even if no external IPv6 address is available?).
[Edit: Search suggests that IPv6 loopback is always available if device supports IPv6.]
OneBlue
left a comment
There was a problem hiding this comment.
Added a couple comments, I would also recommend:
- Updating the pull request title
- Adding test coverage for the binding address wiring (at least for localhost, since that's the only thing that the service supports today)
You'll probably have to update existing tests as well
|
Hey @1wizkid 👋 — Following up on this PR. Currently the wslc tests are failing in CI, and there are 11 unresolved review threads with feedback from maintainers, including:
Is this change still needed? The review feedback covers some important safety and design issues that should be addressed before this can move forward. Let us know if you need any help! |
|
Ill focus on this today.
Thanks,
Richard
Sent from my T-Mobile 5G Device
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
|
Pull request was closed
7f1eca4 to
715ce52
Compare
…ate windowAddress values if present.
…that convertedPorts must stay in same scope as containerOptions and moved the declaration to be with that var
…for Ipv4 and Ipv6 local host.
…log reports easier.
|
The bulk of the issues addressed and new tests submitted as well that cover the usage of windowsAddress IPv4 & IPv6.
Waiting for the test pass to complete and will see if anything left to do.
Thanks,
Richard
|
| HRESULT InetNtopToHresult(int af, const void* src, char* dst, size_t dstCount) | ||
| { | ||
| const char* result = inet_ntop(af, src, dst, dstCount); | ||
| if (result) |
There was a problem hiding this comment.
I recommend throwing the win32 error directly:
THROW_WIN32_IF(WSAGetLastError(), inet_ntop(af, src, dst, dstCount) == nullptr);
|
|
||
| default: | ||
| // Reject unsupported or malformed address families | ||
| THROW_HR(E_INVALIDARG); |
There was a problem hiding this comment.
nit: I recommend logging the address family to make debugging easier
| // Validate IP address if provided and if valid, copy to runtime structure. | ||
| if (internalPort.windowsAddress != nullptr) | ||
| { | ||
| char addrBuf[INET6_ADDRSTRLEN]{}; |
There was a problem hiding this comment.
Instead of having a temporary buffer, I recommend copying directly to convertedPort.BindingAddress
| if (portMappings[i].windowsAddress != nullptr) | ||
| { | ||
| const auto family = portMappings[i].windowsAddress->ss_family; | ||
| RETURN_HR_IF(E_INVALIDARG, family != AF_INET && family != AF_INET6); |
There was a problem hiding this comment.
nit: I recommend logging the invalid address family here
Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed